-
Notifications
You must be signed in to change notification settings - Fork 154
ci: Add initial nemo-lm boilerplate #9
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
Signed-off-by: Charlie Truong <[email protected]>
Signed-off-by: Charlie Truong <[email protected]>
Signed-off-by: Charlie Truong <[email protected]>
Signed-off-by: Charlie Truong <[email protected]>
ko3n1g
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the initial PR! Great work so far, just some small notes and observations
| # limitations under the License. | ||
| name: ~Build container template | ||
| on: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think a copy-paste error
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ah yup.
| --mount=type=bind,source=requirements,target=/workspace/requirements \ | ||
| pip install --no-build-isolation --no-cache-dir '.[all]' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We'll need another layer with devel version of Mcore and NeMo-Run
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there any reason why we shouldn't just update the requirements file to install from a commit if necessary?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
At least for Mcore, I feel that one we could and should be pinning by commit in the requirements file. No different than what we're doing in the current NeMo repo.
For NeMo-Run, maybe we should just be pinning that as well? In both cases, I'm not sure we care too much about staying on main and prefer more stability anyway.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When a requirements file specifies a VCS/git package, the package itself cannot be published to PyPi anymore. We would need to update the requirements-file prior to pushing to PyPi. I was briefly considering to hack around this with extra_requires ([dev]), but I think those fall under the same constraint
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah gotcha. Ok, I'll add that additional layer. Will add build args to enable busting the cache as needed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ko3n1g I updated to add that extra layer in the docker file. Afterwards we can check in with what mcore commits at least the nemo-lm team was developing against. Please take a look when you get a chance.
Signed-off-by: Charlie Truong <[email protected]>
ko3n1g
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Awesome! Please remember to update the test-template action. Other LGTM, approving now to unblock the team
Signed-off-by: ashors1 <[email protected]>
Add initial nemo-lm boilerplate
Will work on ensuring the CI tests run in the next PR.